-
Notifications
You must be signed in to change notification settings - Fork 319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve support for Apple Silicon and macOS #275
base: master
Are you sure you want to change the base?
Conversation
This overlaps with #273 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this seems good, well past our baseline target of just compiling on M1. If you can address my feedback (and verify that things still work on your M1) then we can check this PR on other platforms and land it.
@@ -911,6 +919,8 @@ struct Platform { | |||
struct CpuArmv5 : CpuArm {}; | |||
struct CpuArmv6 : CpuArm {}; | |||
struct CpuArmv7 : CpuArm {}; | |||
struct CpuArmv8 : CpuArm {}; | |||
struct CpuArmv9 : CpuArm {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v9 seems unused, we can remove that.
@@ -448,114 +448,57 @@ struct MachTimerUtil { | |||
|
|||
private: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fairly extensive. Looking at it I think it's fine, but needs a few changes (which i can't comment on directly because of github):
-
I suspect the include of <mach/mach_time.h> is no longer the right include... it looks like it should just be <time.h>?
-
Please change "MachTimerUtil" to "DarwinTimerUtil".
It seems based on documentation that this should also work on non-m1 darwin machines, but we'll have to try to verify that.
@@ -121,7 +121,7 @@ static u64 u8to64_le(const u8* p) | |||
{ | |||
BSLS_ASSERT(p); | |||
|
|||
#if defined(BSLS_PLATFORM_CPU_X86) || defined(BSLS_PLATFORM_CPU_X86_64) | |||
#if defined(BSLS_PLATFORM_IS_LITTLE_ENDIAN) | |||
return *reinterpret_cast<const u64 *>(p); // Ignore alignment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about alignment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, seems AArch64 typically allows unaligned accesses in user space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that is a good point - i wouldn't widen the existing assumption to all little endian machines.
@@ -380,7 +380,7 @@ SpookyHashAlgorithm::SpookyHashAlgorithm() | |||
inline | |||
SpookyHashAlgorithm::SpookyHashAlgorithm(const char *seed) | |||
: d_state( | |||
#if !defined(BSLS_PLATFORM_CPU_X86_64) && !defined(BSLS_PLATFORM_CPU_X86) | |||
#if !defined(BSLS_PLATFORM_IS_LITTLE_ENDIAN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also not be checking for little_endian - this is about whether the cpu handles unaligned reads properly. For now i would leave this as-is and we can engage the optimization at a later date if we make the m1 a more fully supported platform.
Built from this branch on my Mac Mini M1. All I needed to do to get I had an almost completely different set of failed unit tests. Click to see the diff
Platform: macOS Monterey Version 12.1, Mac mini (M1, 2020)
|
Will file internal issue - this was done on my personal M1 laptop
clock_gettime_nsec_np
overmach_absolute_time
Testing performed
bsls_unspecifiedbool.t.cpp
fails to build at allFailing unit tests
bsls_unspecifiedbool.t.cpp build